-
Notifications
You must be signed in to change notification settings - Fork 50
[AQUA] CLI to verify policies to perform operations related to AQUA. #1218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Could you add description to this PR? |
|
|
|
UNVERIFIED = "UNVERIFIED" | ||
|
||
|
||
@dataclass(repr=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: We are trying to move everything to pydantic classes instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These classes are inheriting from DataClassSerializable. Do we have something similar with Pydantic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap we do have
https://github.com/oracle/accelerated-data-science/blob/main/ads/aqua/config/utils/serializer.py
utils.list_compartments.__name__: { | ||
"name": "List Compartments", | ||
"error": "Unable to retrieve the list of compartments. Please verify that you have the required permissions to list compartments in your tenancy. ", | ||
"policy_hint": "Allow dynamic-group aqua-dynamic-group to inspect compartments in tenancy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed during the syncup, let's also try to add a link to the GH docs, where the policies can be setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a log: logger.info(f"Refer to: {POLICY_HELP_LINK}")
This log will be printed everytime when a operation fails.
POLICY_HELP_LINK: https://github.com/oracle-samples/oci-data-science-ai-samples/blob/main/ai-quick-actions/policies/README.md
subnet_id = kwargs.pop("subnet_id", None) | ||
job_infrastructure_type = "STANDALONE" if subnet_id is not None else "ME_STANDALONE" | ||
|
||
response = self.aqua_model.ds_client.create_job( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: This could be done using ADS Jobs implementation.
https://accelerated-data-science.readthedocs.io/en/latest/user_guide/jobs/run_python.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In current implementation, I am creating Job with shell environment and running a simple "echo" command. I am directly uploading the artifact data without saving the file locally.
In job run: I am waiting for run to complete, by constantly monitoring lifecycle state.
I found ads.job
little untidy to achieve the same.
Will the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some minor comments
using Oracle Accelerated Data Science (ADS) SDK. | ||
""" | ||
def __init__(self): | ||
self.aqua_model = AquaModelApp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: to keep it better readable/maintainable, might be good to load most the below clients directly from AquaApp class, and use aqua_model for testing individual functions. ideally, clients should have been reused in the child classes in aqua, but we've done it on few instances and not all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the list operations use limit=3, so they fetch data directly from the OCI client. If I change the implementation to ads specific, it tries to list all resources even if it has multiple pages. For policy verification, a list operation with a limit is enough.
ads/aqua/verify_policies/utils.py
Outdated
model_id = self.aqua_model.ds_client.create_model(create_model_details=create_model_details).data.id | ||
self.aqua_model.ds_client.create_model_artifact( | ||
model_id=model_id, | ||
model_artifact=b"7IV6cktUGcHIhur4bXTv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use a constant for this, also add comment on what this string means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to CONSTANT variable. This is dummy model_artifact bytes.
self.job_id = None | ||
self.limit = 3 | ||
|
||
def list_compartments(self, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add docstrings for all functions.
log.txt
Outdated
@@ -0,0 +1,24 @@ | |||
👋 Waiting, Amit! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this file need to be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
…celerated-data-science into ODSC-72417/Policy_Verification
|
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍
This utility is designed to help users verify whether they have the necessary permissions to perform common AQUA workflows, such as model registration, deployment, evaluation, and fine-tuning.
Common Methods:
ads aqua verify_policies common_policies
ads aqua verify_policies model_register
ads aqua verify_policies model_deployment
ads aqua verify_policies evaluation
ads aqua verify_policies finetune
Help:
ads aqua verify_policies -- --help
Usage: 'ads aqua' verify_policies <command|value>
available commands: common_policies | evaluation | finetune |
model_deployment | model_register
available values: model_id